Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: IATR empty states #25219

Merged
merged 19 commits into from
Dec 22, 2022
Merged

feat: IATR empty states #25219

merged 19 commits into from
Dec 22, 2022

Conversation

astone123
Copy link
Contributor

@astone123 astone123 commented Dec 20, 2022

User facing changelog

n/a - merging to feature branch

Additional details

This PR adds the following empty states to the debug page

  • Not logged in
  • No project connected
  • No runs recorded
  • Loading
  • Error (note that this state is simply triggered by a boolean prop and has a hard-coded message. we will wire this up later)

Steps to test

Look at the new Percy snapshots and compare them with the Figma mockups. Look at the DebugContainer component test and verify that the logic around each state is correct (this was mostly implemented prior to this PR so it should be mostly the same)

How has the user experience changed?

See the new Percy snapshots

PR Tasks

  • Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@astone123 astone123 self-assigned this Dec 20, 2022
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 20, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Dec 20, 2022

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some basic comments, I'm still setting up my Cloud environment locally. I wasn't able to run this to test it, yet.

If you could sync up and help me run this locally, that would be great.

packages/app/src/debug/DebugContainer.vue Outdated Show resolved Hide resolved
packages/app/src/debug/DebugEmptyView.vue Outdated Show resolved Hide resolved
packages/graphql/schemas/cloud.graphql Outdated Show resolved Hide resolved
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far!

packages/app/src/pages/Debug.vue Show resolved Hide resolved
@astone123 astone123 marked this pull request as ready for review December 20, 2022 22:56
@astone123 astone123 requested review from warrensplayer and a team December 20, 2022 22:57
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and works great! I'v got the one comment regarding the transition that I would like to see addressed.

packages/app/src/debug/DebugContainer.vue Show resolved Hide resolved
Copy link
Contributor

@mike-plummer mike-plummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshots look 🔥 , nice work. Few comments/questions. Also seeing some Percy changes that don't seem related, but there doesn't appear to be any code that would account for them (selector playground button shifted a couple pixels, button widths changing). Assuming those are flake?

cy.contains('button', 'Log in to Cypress Cloud').click()
cy.contains('button', 'Connect to Cypress Cloud').click()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: updating from "Log in" to "Connect" - there were conversations around this back when we put this in for ACI, concern then was that the top-level "Log In" button in the upper-right would have the same action so this button should use the same wording.

I'm assuming this was an update driven by design? If so then okay, just felt weird to see this get reversed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I talked to Emil about this - I think this should only affect the wording for the button on the runs page

packages/app/src/debug/DebugContainer.cy.tsx Show resolved Hide resolved
})

it('is logged in', () => {
it('is logged in with no project', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ACI there were three separate "no project" states - are these all being rolled into this single "no project connected" condition?

  • No projectId at all
  • Invalid projectId (bad value, project since deleted, etc)
  • Unauthorized (valid projectId but user doesn't have access)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I believe here we're just doing one view that is based on loginConnectStore.project.isProjectConnected. @warrensplayer can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astone123 I am going to check with Peter and I will file a follow up issue if we need to add these additional states. I do not want to increase the scope of this issue beyond the original requirements.

packages/app/src/debug/DebugContainer.cy.tsx Outdated Show resolved Hide resolved
packages/app/src/debug/DebugContainer.cy.tsx Outdated Show resolved Hide resolved
packages/app/src/debug/DebugEmptyView.vue Outdated Show resolved Hide resolved
packages/frontend-shared/src/locales/en-US.json Outdated Show resolved Hide resolved
packages/app/src/debug/DebugLoading.vue Outdated Show resolved Hide resolved
packages/app/src/debug/DebugLoading.vue Outdated Show resolved Hide resolved
packages/app/src/debug/DebugLoading.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@mike-plummer mike-plummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments have been addressed 🚀

Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks super! I addressed your question that Mike brought up about those other states. We can address in another issue. My only other comment is nit-pick around code organization. It would be nice to group some of your similar components together in subdirectories. I think you can put all your empty state components in an emptyStates folder along with the one CT test. Going to go ahead and approve.

@@ -292,6 +297,7 @@ export const CloudRunStubs = {
somePending: createCloudRun({ status: 'PASSED', totalPassed: 100, totalSkipped: 3000, totalPending: 20 }),
allSkipped: createCloudRun({ status: 'ERRORED', totalPassed: 0, totalSkipped: 10 }),
failingWithTests: addFailedTests(createCloudRun({ status: 'FAILED', totalPassed: 8, totalFailed: 2 })),
errored: createCloudRun({ status: 'ERRORED', totalPassed: 1, totalFailed: 0, errors: ['There was an error'] }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this anymore? It does not look like it is being used.

})

it('is logged in', () => {
it('is logged in with no project', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astone123 I am going to check with Peter and I will file a follow up issue if we need to add these additional states. I do not want to increase the scope of this issue beyond the original requirements.

Comment on lines 17 to 18
<DebugLoadingDivider />
<div class="rounded-full bg-gray-50 h-8px ml-8px w-80px" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<DebugLoadingDivider />
<div class="rounded-full bg-gray-50 h-8px ml-8px w-80px" />
<template v-for="n in 4">
<DebugLoadingDivider />
<div class="rounded-full bg-gray-50 h-8px ml-8px w-80px" />
</template>

Nit-pick: you can repeat this same content 4 times using this v-for syntax https://vuejs.org/guide/essentials/list.html#v-for-with-a-range

There are other places in this file you could do that with as well.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few minor nits from @warrensplayer. My comments are ✔️. I will leave fixing + merging to you @astone123. 💯

@astone123 astone123 merged commit 552fe87 into feature/IATR-M0 Dec 22, 2022
@astone123 astone123 deleted the astone123/IATR-empty-states branch December 22, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants